Speed up Constraint Solver's overload resolution (II) #1650
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Commit constraint solution before the type equality test. * SolveMemberConstraint will signal through an internal exception that it failed due to an unresolved overload.
|
The only test that was failing is the example added by @forki on #1530 which was never finishing to compile before that PR, then after #1530 it fails with "No overloads match". Now with this PR it will fail with "This value is not a function ..." which could makes sense by reading the code, since it's a So this could be an example of a bug fix mentioned above in my PR description. Before it was considering a false positive which gave more than one valid overload, that's why it was failing with "No overloads match". |
|
This change looks great. Approved. |
dsyme
approved these changes
Nov 1, 2016
|
@dsyme I found a small issue with this PR but it's fixed now. The original reasoning was to observe the result only when calling from Intuition and extensive testing showed that To avoid that, we need to pass an additional parameter which was my original reasoning, and that's exactly what I did to fix it. |
|
Thank you for taking care of this |
KevinRansom
pushed a commit
that referenced
this pull request
Jan 4, 2018
forki
pushed a commit
to forki/visualfsharp
that referenced
this pull request
Jan 20, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074) * Add option to toggle unused declarations analyzer * Better name and handle registering code fixes. This will ensure that if someone uses warnon:1182, we won't suggest fixes if they've turned off the feature. * Revert dotnet#1650 (and dotnet#3366) (dotnet#4173) * Fix error logging in brace matching code (dotnet#4140) * Remove error logger pushing code * Update service.fs * Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202) * LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194) * Fixed FCS netcore tests (dotnet#4180) * Remove ambiguous resolution error FS0332 (dotnet#4170) * Add IsInteractive to parsing options for script load closures (dotnet#4169) * Add IsInteractive to FSharpParsingOptions * Add test * Set serializable bit for all serializable types (dotnet#4211) * Minor fix (dotnet#4195) on string 58. * Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222) * Symbols API: add Index to active pattern case, Name to pattern group * Symbols API: add active pattern case use tests * don't rebuild (dotnet#4230) * Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests
ncave
added a commit
to ncave/fsharp
that referenced
this pull request
Jan 24, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074) * Add option to toggle unused declarations analyzer * Better name and handle registering code fixes. This will ensure that if someone uses warnon:1182, we won't suggest fixes if they've turned off the feature. * Revert dotnet#1650 (and dotnet#3366) (dotnet#4173) * Fix error logging in brace matching code (dotnet#4140) * Remove error logger pushing code * Update service.fs * Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202) * LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194) * Fixed FCS netcore tests (dotnet#4180) * Remove ambiguous resolution error FS0332 (dotnet#4170) * Add IsInteractive to parsing options for script load closures (dotnet#4169) * Add IsInteractive to FSharpParsingOptions * Add test * Set serializable bit for all serializable types (dotnet#4211) * Minor fix (dotnet#4195) on string 58. * Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222) * Symbols API: add Index to active pattern case, Name to pattern group * Symbols API: add active pattern case use tests * don't rebuild (dotnet#4230) * Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests
ncave
added a commit
to ncave/fsharp
that referenced
this pull request
Jan 24, 2018
* Add option to toggle unused declarations analyzer (dotnet#4074) * Add option to toggle unused declarations analyzer * Better name and handle registering code fixes. This will ensure that if someone uses warnon:1182, we won't suggest fixes if they've turned off the feature. * Revert dotnet#1650 (and dotnet#3366) (dotnet#4173) * Fix error logging in brace matching code (dotnet#4140) * Remove error logger pushing code * Update service.fs * Fix dotnet#4200: Vsix: fix empty "New file" window for web projects (dotnet#4202) * LOC CHECKIN | visualfsharp - master | 20180112 (dotnet#4194) * Fixed FCS netcore tests (dotnet#4180) * Remove ambiguous resolution error FS0332 (dotnet#4170) * Add IsInteractive to parsing options for script load closures (dotnet#4169) * Add IsInteractive to FSharpParsingOptions * Add test * Set serializable bit for all serializable types (dotnet#4211) * Minor fix (dotnet#4195) on string 58. * Symbols API: add Index to active pattern case, Name to pattern group (dotnet#4222) * Symbols API: add Index to active pattern case, Name to pattern group * Symbols API: add active pattern case use tests * don't rebuild (dotnet#4230) * Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests
forki
added a commit
to forki/visualfsharp
that referenced
this pull request
Jan 31, 2018
This reverts commit f9893b6.
dsyme
added a commit
to dsyme/fsharp
that referenced
this pull request
Feb 13, 2018
This reverts commit f9893b6.
dsyme
added a commit
to dsyme/fsharp
that referenced
this pull request
Feb 13, 2018
This reverts commit f9893b6.
KevinRansom
pushed a commit
that referenced
this pull request
Feb 17, 2018
* Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests (#40) * Add option to toggle unused declarations analyzer (#4074) * Add option to toggle unused declarations analyzer * Better name and handle registering code fixes. This will ensure that if someone uses warnon:1182, we won't suggest fixes if they've turned off the feature. * Revert #1650 (and #3366) (#4173) * Fix error logging in brace matching code (#4140) * Remove error logger pushing code * Update service.fs * Fix #4200: Vsix: fix empty "New file" window for web projects (#4202) * LOC CHECKIN | visualfsharp - master | 20180112 (#4194) * Fixed FCS netcore tests (#4180) * Remove ambiguous resolution error FS0332 (#4170) * Add IsInteractive to parsing options for script load closures (#4169) * Add IsInteractive to FSharpParsingOptions * Add test * Set serializable bit for all serializable types (#4211) * Minor fix (#4195) on string 58. * Symbols API: add Index to active pattern case, Name to pattern group (#4222) * Symbols API: add Index to active pattern case, Name to pattern group * Symbols API: add active pattern case use tests * don't rebuild (#4230) * Optimize in FCS * Transport tcConfig * Cleanup * Replace more ILAsm in Exprs * More ILAsm replacements * update resource name * Added some tests * test conditions update * test update * test condition update * test update * review update * added checked operators * fixed dual conversions * review fixes * more targeted replacements * adapt to latest * added more tests * added more tests * review fixes * fixed warnings
This was referenced Sep 27, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will speed up to 2x overload resolution in cases like this.
This PR is a follow-up of #1530 where it was discussed that ideally the constraint solution can be committed a bit early.
The problem was that knowing the solution early would make some very exotic cases like this one to fail.
After analyzing the problem I found that cases like that would fail because the first overload that is considered would succeed if the constraint is assumed as solved, when in fact it should fail (false positive).
Assuming that specific constraint solution makes impossible to solve the nested overload resolution called by
SolveMemberConstraintwhich is expected to fail and indeed it fails, but because that result is ignored, it's marked as a false positive and the compilation breaks later by not being able to decide between 2 overloads: the good one and the bad one which is treated as good.The proposed change adds an internal exception to signal the calling function (in the analysed case it was called from
solveTypMeetsTyparConstraints) an unresolved overload and make the whole assumption fail.The tests I made show no performance degradation but an improvement in some cases, which is logically expected because we will be using a shortcut in some cases.
But the improvement went a bit further since now by signaling an inconsistent case it also discards early some paths and I wouldn't be surprised if it fixes some existing bugs which were consequence of this previously ignored result.
Here's a test I run with the following results: